Automated Test: rest-serializer-enhancement-post #316
Conversation
📝 WalkthroughWalkthroughThis pull request implements a new admin feature for managing embeddable hosts. It replaces SiteSetting-based host validation with a database-backed EmbeddableHost model, adding admin UI for creating, editing, and deleting allowed hosts with category associations. Changes span frontend (Ember components, routes, templates), backend (Rails controllers, models, serializers), database schema, and configuration. Changes
Sequence DiagramsequenceDiagram
actor Admin
participant AdminUI as Admin UI<br/>(Ember)
participant Controller as EmbeddingController<br/>(Rails)
participant Model as EmbeddableHost<br/>(Model)
participant DB as Database
Admin->>AdminUI: Visit /admin/customize/embedding
AdminUI->>Controller: GET /admin/customize/embedding
Controller->>Model: EmbeddableHost.all
Model->>DB: Query hosts
DB-->>Model: Return hosts
Model-->>Controller: [hosts]
Controller-->>AdminUI: Render embedding with hosts
Admin->>AdminUI: Click "Add Host"
AdminUI->>AdminUI: Create new host record
Admin->>AdminUI: Enter host + category
Admin->>AdminUI: Click Save
AdminUI->>Controller: POST /admin/embeddable_hosts
Controller->>Model: Create & validate host
Model->>DB: Insert record
DB-->>Model: Success
Model-->>Controller: Serialized host
Controller-->>AdminUI: Updated host
AdminUI->>AdminUI: Update list
Admin->>AdminUI: Click Delete host
AdminUI->>AdminUI: Confirm deletion
AdminUI->>Controller: DELETE /admin/embeddable_hosts/:id
Controller->>Model: Destroy host
Model->>DB: Delete record
DB-->>Model: Success
Model-->>Controller: OK
Controller-->>AdminUI: Success
AdminUI->>AdminUI: Remove from list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This change introduces a new feature spanning multiple layers (frontend components, Rails controllers, models, migrations, serializers, tests), with moderate logic density in host validation, category associations, and data migration. The fabricator file swap and multiple related test updates add heterogeneity requiring careful review of each component's interactions. Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Fix all issues with AI agents
In `@app/assets/javascripts/admin/components/embeddable-host.js.es6`:
- Around line 43-51: The delete() action currently calls
this.get('host').destroyRecord() without error handling; update delete() to
attach a .catch handler that calls popupAjaxError (same error handler used by
save) and ensure the promise chain still triggers sendAction('deleteHost',
this.get('host')) only on success; specifically modify the
destroyRecord().then(...) to destroyRecord().then(() => {
this.sendAction('deleteHost', this.get('host')); }).catch(popupAjaxError) so
server errors are surfaced to the user.
In `@app/assets/javascripts/admin/controllers/admin-embedding.js.es6`:
- Around line 5-6: The saveChanges method calls this.get('embedding').update({})
but doesn't handle the returned promise; update saveChanges to attach a
.catch(...) that surfaces errors (e.g., show a flash/notification) and import
the app's error handler (popupAjaxError or equivalent) at the top of the file;
specifically, locate the saveChanges method and change the call to
this.get('embedding').update({}).catch(err => popupAjaxError(err)) (or use your
app's notification API) and add the popupAjaxError import to ensure failures are
not silently swallowed.
In `@app/assets/javascripts/discourse/adapters/rest.js.es6`:
- Line 22: The lookup uses type.replace('_', '-') which only replaces the first
underscore so ADMIN_MODELS membership checks can fail for names with multiple
underscores; update the expression used in the membership test (the call to
type.replace('_', '-')) to perform a global replace (e.g., use a regex with the
global flag) so all underscores are replaced before checking ADMIN_MODELS,
leaving the surrounding if and return logic unchanged.
In `@app/assets/javascripts/discourse/models/store.js.es6`:
- Around line 192-209: For the multi-id branch, avoid producing undefined
entries and avoid unconditionally deleting the source _ids key: first collect
the ids (const ids = obj[k]) and map to hydrated = ids.map(id =>
self._lookupSubType(subType, type, id, root)); remove the dead "|| []" and do
not assign undefined entries — set obj[self.pluralize(subType)] to
hydratedFiltered = hydrated.filter(h => h != null); only delete the original
obj[k] if every lookup succeeded (if hydrated.every(h => h != null)), otherwise
keep obj[k] as a fallback while assigning the filtered list.
In `@app/controllers/admin/embeddable_hosts_controller.rb`:
- Around line 9-18: The update and destroy actions call methods on a potentially
nil record because they use EmbeddableHost.where(id: params[:id]).first; change
these to use EmbeddableHost.find(params[:id]) so Rails will raise
ActiveRecord::RecordNotFound (rendering a 404) or alternatively add a nil guard
that renders a 404 when the lookup returns nil before calling save_host or
host.destroy; update the update method (which calls save_host) and the destroy
method to use one of these two approaches.
In `@app/controllers/admin/embedding_controller.rb`:
- Around line 9-11: The update action currently just re-renders `@embedding` and
ignores request params; change it to apply strong params to `@embedding` (e.g.
call an embedding_params/permit method), attempt to update and persist (use
`@embedding.update`(embedding_params) or equivalent), and then
render_serialized(`@embedding`, EmbeddingSerializer, root: 'embedding',
rest_serializer: true) on success or return appropriate error responses
(validation errors / 422) on failure; also add or reuse a private
embedding_params method to whitelist allowed attributes.
In `@app/models/embeddable_host.rb`:
- Around line 5-8: The before_validation block calls self.host.sub! which raises
NoMethodError when host is nil; update the before_validation callback to guard
against nil by returning early or only calling sub! when host is present (e.g.,
check host.present? or host && before mutating), so the two substitutions in the
block (self.host.sub!(/^https?:\/\//, '') and self.host.sub!(/\/.*$/, '')) are
only invoked when host is not nil.
- Line 2: The host validation regex in validates_format_of :host currently caps
TLDs with `[a-z]{2,5}` which rejects longer valid gTLDs; update the regex in
embeddable_host.rb (the validates_format_of :host line) to allow longer TLDs
such as `[a-z]{2,}` (or a bounded range like `{2,24}`) so modern TLDs pass
validation, keeping the rest of the pattern intact and preserving the
case-insensitive flag.
In `@app/models/topic_embed.rb`:
- Around line 36-43: TopicEmbed.import currently creates posts even when
EmbeddableHost.record_for_host(url) returns nil, allowing unknown hosts through
RSS imports; add an early guard in TopicEmbed.import right after eh =
EmbeddableHost.record_for_host(url) to reject unknown/unallowed hosts (e.g.,
return unless eh.present? or check EmbeddableHost.host_allowed?(url)) so that
PollFeed.import_topic and other callers cannot bypass the host whitelist before
calling PostCreator.
In `@db/migrate/20150818190757_create_embeddable_hosts.rb`:
- Around line 24-26: The INSERT currently interpolates host values inside the
raw SQL in the records.each block (the execute call), which allows SQL injection
for values with quotes; fix it by quoting the host value via
ActiveRecord::Base.connection.quote (or connection.quote) before building the
SQL string and use that quoted value in the execute call so the SQL is safe and
handles embedded quotes.
- Around line 9-16: The migration assumes queries for the 'embed_category' and
'uncategorized_category_id' site settings always return rows; instead, guard the
results from execute(...) before indexing into [0]['id'] or [0]['value'.] For
the first query (the one using INNER JOIN and 'embed_category') assign its
result to a local variable (e.g. embed_res), check embed_res.present? or
embed_res.any? before reading embed_res[0]['id'].to_i and only then set
category_id; if it's nil, run the fallback query similarly into
uncategorized_res, check that result before reading ['value'] and convert to_i,
and if both are missing, raise a clear migration error or set a safe
default—update the code around the category_id assignment and the two
execute(...) calls to perform these nil-guards.
In `@spec/fabricators/category_fabricator.rb`:
- Around line 1-4: The file names and contents are swapped:
spec/fabricators/category_fabricator.rb currently defines
Fabricator(:embeddable_host) while
spec/fabricators/embeddable_host_fabricator.rb defines the :category
fabricators; fix this by moving the Fabricator(:embeddable_host) block into
embeddable_host_fabricator.rb and moving the :category fabricators into
category_fabricator.rb (or rename files to match the contents), ensuring
Fabricator(:embeddable_host) lives in embeddable_host_fabricator.rb and the
:category Fabricator(s) live in category_fabricator.rb.
In `@spec/models/topic_spec.rb`:
- Line 1407: The test description string in the describe block currently
contains a typo: change "with an emeddable host" to "with an embeddable host" in
the describe(...) line in spec/models/topic_spec.rb so the spec reads 'describe
"with an embeddable host" do'.
In `@test/javascripts/helpers/create-pretender.js.es6`:
- Around line 226-229: The GET handler for '/fruits/:id' always returns
fruits[0] instead of the requested record; change the handler function signature
to accept the request, extract the id from request.params (e.g. const id =
parseInt(request.params.id, 10)), find the matching fruit (e.g. const fruit =
fruits.find(f => f.id === id) || fruits[0]) and return that in response({
__rest_serializer: "1", fruit, farmers, colors }); then update the corresponding
test assertions in store-test.js.es6 to expect the data for the requested fruit
ID (e.g. id 2) rather than apple's data.
In `@test/javascripts/models/store-test.js.es6`:
- Around line 109-121: The test "find embedded" calls store.find('fruit', 2) but
the pretender mock always returns fruits[0] (id 1), so update the test or the
mock: either change the test's call to store.find('fruit', 1) so it matches the
current mock response, or modify the pretender handler that serves the /fruits
endpoint to look up the requested id and return the matching entry from the
fruits array; adjust references in the test around colors and category only if
you change the returned fruit.
🧹 Nitpick comments (14)
db/migrate/20150818190757_create_embeddable_hosts.rb (1)
1-32: Migration useschangebut contains irreversible raw SQL (INSERT, DELETE).The
changemethod implies reversibility, but the raw SQLINSERTandDELETEoperations cannot be auto-reversed by ActiveRecord. A rollback will fail. Consider usingup/downmethods or wrapping the irreversible parts inreversibleblocks with explicitdownlogic.spec/controllers/admin/embeddable_hosts_controller_spec.rb (1)
3-7: Test coverage is minimal — consider adding functional tests.This spec only verifies inheritance. The
Admin::EmbeddableHostsControllerlikely hascreate,update, anddestroyactions that should be covered by request/controller specs to validate authorization, input handling, and error paths.Would you like me to generate controller specs for the CRUD actions, or open an issue to track this?
spec/controllers/admin/embedding_controller_spec.rb (1)
3-9: Minimal test coverage — consider adding specs forshowandupdateactions.The inheritance check is fine, but the controller has
showandupdateactions (per the AI summary) that aren't exercised here. TheAdmin::EmbeddableHostsControllerspec apparently covers CRUD; consider parity for this controller.Also, RuboCop flags extra empty lines at the block body beginning (line 4) and end (line 8):
♻️ Formatting fix
describe Admin::EmbeddingController do - it "is a subclass of AdminController" do expect(Admin::EmbeddingController < Admin::AdminController).to eq(true) end - endapp/assets/javascripts/admin/templates/embedding.hbs (1)
4-6: Consider using CSS classes instead of inlinestyleattributes for column widths.Inline styles are harder to maintain and override. A small admin-specific CSS class would be cleaner, though this is low priority for an admin-only template.
app/serializers/embeddable_host_serializer.rb (1)
4-14: Redundant accessor methods —attributesalready delegates toobject.The
attributes :id, :host, :category_iddeclaration automatically generates accessors that delegate toobject.id,object.host, andobject.category_id. These explicit method definitions can be safely removed.Proposed simplification
class EmbeddableHostSerializer < ApplicationSerializer attributes :id, :host, :category_id - - def id - object.id - end - - def host - object.host - end - - def category_id - object.category_id - end endspec/models/embeddable_host_spec.rb (2)
3-40: RuboCop: extra empty lines at block body boundaries.Static analysis flags extra blank lines at the beginning of the outer
describeblock (after Line 4) and at the end of the innerdescribeblock (after Line 37) and outer block (after Line 39).Fix formatting
describe EmbeddableHost do - it "trims http" doend - end - end
23-37: Consider adding edge-case tests forhost_allowed?.The happy-path and negative-case coverage is solid. You might also consider testing:
nilinput → should returnfalse- Empty string → should return
false- Host with a port (e.g.,
http://eviltrout.com:8080) → verify behavior- Subdomain mismatch (e.g.,
http://sub.eviltrout.com) → verify it's deniedThese would help ensure the model's
record_for_hostmethod handles edge cases gracefully.app/models/embeddable_host.rb (2)
10-18: Variable shadowing and missingdowncaseon query input.Line 14 shadows the method parameter
hostwithuri.host. More importantly, Line 17 useslower(host)on the DB column but doesn't downcase the Ruby-sidehostvalue. WhileURI#hosttypically returns lowercase, this isn't guaranteed for all inputs. Add.downcaseto ensure a consistent match.Proposed fix
def self.record_for_host(host) uri = URI(host) rescue nil return false unless uri.present? - host = uri.host - return false unless host.present? + extracted_host = uri.host + return false unless extracted_host.present? - where("lower(host) = ?", host).first + where("lower(host) = ?", extracted_host.downcase).first end
11-11: Avoid inlinerescuemodifier (RuboCop:Style/RescueModifier).Inline
rescuecatches allStandardErrorsubclasses silently, which can mask unexpected errors. Per the static analysis hint, use an explicitbegin/rescueblock or a safe parse method.Proposed fix
- uri = URI(host) rescue nil + begin + uri = URI(host) + rescue URI::InvalidURIError + return false + endspec/fabricators/embeddable_host_fabricator.rb (1)
23-25:update!insideafter_buildprematurely persists the record.Calling
cat.update!(read_restricted: true)withinafter_buildsaves the record before the Fabrication gem performs its own save. Consider usingafter_createinstead, or set the attribute without persisting:Proposed fix using attribute assignment in after_build
after_build do |cat, transients| - cat.update!(read_restricted: true) + cat.read_restricted = true cat.category_groups.build(group_id: transients[:group].id, permission_type: CategoryGroup.permission_types[:full]) endconfig/routes.rb (1)
138-139: New admin routes for embedding and embeddable hosts look correct.The routes are properly scoped within the admin namespace with
AdminConstraint. Note thatresources :embeddable_hostson Line 153 generates all 7 RESTful routes, but the controller only implementscreate,update, anddestroy. The unused routes (index,show,new,edit) will return errors if hit. Consider restricting withonly:if these actions aren't needed:- resources :embeddable_hosts, constraints: AdminConstraint.new + resources :embeddable_hosts, constraints: AdminConstraint.new, only: [:create, :update, :destroy]Also applies to: 153-153
app/controllers/admin/embeddable_hosts_controller.rb (1)
22-32: No strong parameters; directparamshash access is fragile.
params[:embeddable_host][:host]will raiseNoMethodErrorif the:embeddable_hostkey is missing from the request. Consider using strong parameters for safety and to whitelist permitted attributes:Proposed fix
def save_host(host) - host.host = params[:embeddable_host][:host] - host.category_id = params[:embeddable_host][:category_id] + host.host = host_params[:host] + host.category_id = host_params[:category_id] host.category_id = SiteSetting.uncategorized_category_id if host.category_id.blank? if host.save render_serialized(host, EmbeddableHostSerializer, root: 'embeddable_host', rest_serializer: true) else render_json_error(host) end end + + private + + def host_params + params.require(:embeddable_host).permit(:host, :category_id) + endapp/controllers/admin/embedding_controller.rb (1)
3-3: Replacebefore_filterwithbefore_actionand remove redundant filters.
Admin::AdminControlleralready enforcesensure_logged_inandensure_staffviabefore_filterdirectives (lines 3–4 in app/controllers/admin/admin_controller.rb), making them redundant here. Additionally,before_filteris deprecated in favor ofbefore_action.Proposed fix
- before_filter :ensure_logged_in, :ensure_staff, :fetch_embedding + before_action :fetch_embeddingapp/assets/javascripts/admin/components/embeddable-host.js.es6 (1)
13-17:@observes('editing')fires even wheneditingbecomesfalse.The observer triggers on every change to
editing, including when exiting edit mode. This unnecessarily schedules a render callback to focus an input that no longer exists. Consider guarding with a check.♻️ Suggested guard
_focusOnInput() { + if (!this.get('editing')) { return; } Ember.run.schedule('afterRender', () => { this.$('.host-name').focus(); }); },
| delete() { | ||
| bootbox.confirm(I18n.t('admin.embedding.confirm_delete'), (result) => { | ||
| if (result) { | ||
| this.get('host').destroyRecord().then(() => { | ||
| this.sendAction('deleteHost', this.get('host')); | ||
| }); | ||
| } | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Missing error handling on destroyRecord().
Unlike the save action which uses .catch(popupAjaxError), the delete action silently swallows server errors. If deletion fails (e.g., 403/500), the user receives no feedback and the host remains in the list without any indication of failure.
🐛 Proposed fix to add error handling
delete() {
bootbox.confirm(I18n.t('admin.embedding.confirm_delete'), (result) => {
if (result) {
this.get('host').destroyRecord().then(() => {
this.sendAction('deleteHost', this.get('host'));
- });
+ }).catch(popupAjaxError);
}
});
},🤖 Prompt for AI Agents
In `@app/assets/javascripts/admin/components/embeddable-host.js.es6` around lines
43 - 51, The delete() action currently calls this.get('host').destroyRecord()
without error handling; update delete() to attach a .catch handler that calls
popupAjaxError (same error handler used by save) and ensure the promise chain
still triggers sendAction('deleteHost', this.get('host')) only on success;
specifically modify the destroyRecord().then(...) to destroyRecord().then(() =>
{ this.sendAction('deleteHost', this.get('host')); }).catch(popupAjaxError) so
server errors are surfaced to the user.
| saveChanges() { | ||
| this.get('embedding').update({}); |
There was a problem hiding this comment.
No error handling on saveChanges.
this.get('embedding').update({}) returns a promise, but failures are silently swallowed. Consider adding a .catch() to surface errors to the user (e.g., via a flash message or notification).
Proposed fix
saveChanges() {
- this.get('embedding').update({});
+ this.get('embedding').update({}).catch(popupAjaxError);
},You'll need to import popupAjaxError (or your app's equivalent error handler) at the top of the file.
🤖 Prompt for AI Agents
In `@app/assets/javascripts/admin/controllers/admin-embedding.js.es6` around lines
5 - 6, The saveChanges method calls this.get('embedding').update({}) but doesn't
handle the returned promise; update saveChanges to attach a .catch(...) that
surfaces errors (e.g., show a flash/notification) and import the app's error
handler (popupAjaxError or equivalent) at the top of the file; specifically,
locate the saveChanges method and change the call to
this.get('embedding').update({}).catch(err => popupAjaxError(err)) (or use your
app's notification API) and add the popupAjaxError import to ensure failures are
not silently swallowed.
| @@ -19,7 +19,7 @@ function rethrow(error) { | |||
| export default Ember.Object.extend({ | |||
|
|
|||
| basePath(store, type) { | |||
| if (ADMIN_MODELS.indexOf(type) !== -1) { return "/admin/"; } | |||
| if (ADMIN_MODELS.indexOf(type.replace('_', '-')) !== -1) { return "/admin/"; } | |||
There was a problem hiding this comment.
String.replace with a string pattern only replaces the first underscore.
type.replace('_', '-') replaces only the first occurrence of _. If a model type with multiple underscores is ever added to ADMIN_MODELS, the lookup will silently fail. Use a regex with the global flag instead.
Suggested fix
- if (ADMIN_MODELS.indexOf(type.replace('_', '-')) !== -1) { return "/admin/"; }
+ if (ADMIN_MODELS.indexOf(type.replace(/_/g, '-')) !== -1) { return "/admin/"; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (ADMIN_MODELS.indexOf(type.replace('_', '-')) !== -1) { return "/admin/"; } | |
| if (ADMIN_MODELS.indexOf(type.replace(/_/g, '-')) !== -1) { return "/admin/"; } |
🤖 Prompt for AI Agents
In `@app/assets/javascripts/discourse/adapters/rest.js.es6` at line 22, The lookup
uses type.replace('_', '-') which only replaces the first underscore so
ADMIN_MODELS membership checks can fail for names with multiple underscores;
update the expression used in the membership test (the call to type.replace('_',
'-')) to perform a global replace (e.g., use a regex with the global flag) so
all underscores are replaced before checking ADMIN_MODELS, leaving the
surrounding if and return logic unchanged.
| const m = /(.+)\_id(s?)$/.exec(k); | ||
| if (m) { | ||
| const subType = m[1]; | ||
| const hydrated = self._lookupSubType(subType, type, obj[k], root); | ||
| if (hydrated) { | ||
| obj[subType] = hydrated; | ||
|
|
||
| if (m[2]) { | ||
| const hydrated = obj[k].map(function(id) { | ||
| return self._lookupSubType(subType, type, id, root); | ||
| }); | ||
| obj[self.pluralize(subType)] = hydrated || []; | ||
| delete obj[k]; | ||
| } else { | ||
| const hydrated = self._lookupSubType(subType, type, obj[k], root); | ||
| if (hydrated) { | ||
| obj[subType] = hydrated; | ||
| delete obj[k]; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Multi-id hydration can silently introduce undefined entries and unconditionally deletes the source key.
Two concerns with the multi-id branch:
-
undefinedin hydrated array: If_lookupSubTypereturnsundefinedfor any ID (e.g., a missing entry inroot), the resulting array will containundefinedelements. Downstream code (including the tests) calls.get('id')on these items, which would throw. -
Asymmetric delete: The single-id branch (line 204-206) only deletes the
_idkey when hydration succeeds, preserving the raw ID as a fallback. The multi-id branch always deletes_ids(line 201) even if hydration producedundefinedentries, losing the original IDs with no fallback.
Additionally, || [] on line 200 is dead code since .map() always returns an array.
Proposed fix: filter out failed lookups and guard the delete
if (m[2]) {
- const hydrated = obj[k].map(function(id) {
- return self._lookupSubType(subType, type, id, root);
- });
- obj[self.pluralize(subType)] = hydrated || [];
+ const hydrated = obj[k].map(function(id) {
+ return self._lookupSubType(subType, type, id, root);
+ }).filter(Boolean);
+ obj[self.pluralize(subType)] = hydrated;
delete obj[k];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const m = /(.+)\_id(s?)$/.exec(k); | |
| if (m) { | |
| const subType = m[1]; | |
| const hydrated = self._lookupSubType(subType, type, obj[k], root); | |
| if (hydrated) { | |
| obj[subType] = hydrated; | |
| if (m[2]) { | |
| const hydrated = obj[k].map(function(id) { | |
| return self._lookupSubType(subType, type, id, root); | |
| }); | |
| obj[self.pluralize(subType)] = hydrated || []; | |
| delete obj[k]; | |
| } else { | |
| const hydrated = self._lookupSubType(subType, type, obj[k], root); | |
| if (hydrated) { | |
| obj[subType] = hydrated; | |
| delete obj[k]; | |
| } | |
| } | |
| const m = /(.+)\_id(s?)$/.exec(k); | |
| if (m) { | |
| const subType = m[1]; | |
| if (m[2]) { | |
| const hydrated = obj[k].map(function(id) { | |
| return self._lookupSubType(subType, type, id, root); | |
| }).filter(Boolean); | |
| obj[self.pluralize(subType)] = hydrated; | |
| delete obj[k]; | |
| } else { | |
| const hydrated = self._lookupSubType(subType, type, obj[k], root); | |
| if (hydrated) { | |
| obj[subType] = hydrated; | |
| delete obj[k]; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@app/assets/javascripts/discourse/models/store.js.es6` around lines 192 - 209,
For the multi-id branch, avoid producing undefined entries and avoid
unconditionally deleting the source _ids key: first collect the ids (const ids =
obj[k]) and map to hydrated = ids.map(id => self._lookupSubType(subType, type,
id, root)); remove the dead "|| []" and do not assign undefined entries — set
obj[self.pluralize(subType)] to hydratedFiltered = hydrated.filter(h => h !=
null); only delete the original obj[k] if every lookup succeeded (if
hydrated.every(h => h != null)), otherwise keep obj[k] as a fallback while
assigning the filtered list.
| def update | ||
| host = EmbeddableHost.where(id: params[:id]).first | ||
| save_host(host) | ||
| end | ||
|
|
||
| def destroy | ||
| host = EmbeddableHost.where(id: params[:id]).first | ||
| host.destroy | ||
| render json: success_json | ||
| end |
There was a problem hiding this comment.
update and destroy crash with NoMethodError when the record is not found.
EmbeddableHost.where(id: params[:id]).first returns nil if no matching record exists. Lines 11 and 16 then call methods on nil, crashing the request. Use find (which raises ActiveRecord::RecordNotFound, returning a 404) or add a nil guard.
Proposed fix
def update
- host = EmbeddableHost.where(id: params[:id]).first
+ host = EmbeddableHost.find(params[:id])
save_host(host)
end
def destroy
- host = EmbeddableHost.where(id: params[:id]).first
+ host = EmbeddableHost.find(params[:id])
host.destroy
render json: success_json
end🤖 Prompt for AI Agents
In `@app/controllers/admin/embeddable_hosts_controller.rb` around lines 9 - 18,
The update and destroy actions call methods on a potentially nil record because
they use EmbeddableHost.where(id: params[:id]).first; change these to use
EmbeddableHost.find(params[:id]) so Rails will raise
ActiveRecord::RecordNotFound (rendering a 404) or alternatively add a nil guard
that renders a 404 when the lookup returns nil before calling save_host or
host.destroy; update the update method (which calls save_host) and the destroy
method to use one of these two approaches.
| records.each do |h| | ||
| execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES ('#{h}', #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)" | ||
| end |
There was a problem hiding this comment.
SQL injection via string interpolation in INSERT statement.
The host value h is directly interpolated into SQL without escaping. Even though the data originates from admin-controlled site_settings, this is unsafe — values containing single quotes will break the query, and malicious content would be executed.
Use connection.quote or ActiveRecord::Base.connection.quote:
Proposed fix
- execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES ('#{h}', #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)"
+ execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES (#{ActiveRecord::Base.connection.quote(h)}, #{category_id.to_i}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| records.each do |h| | |
| execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES ('#{h}', #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)" | |
| end | |
| records.each do |h| | |
| execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES (#{ActiveRecord::Base.connection.quote(h)}, #{category_id.to_i}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)" | |
| end |
🤖 Prompt for AI Agents
In `@db/migrate/20150818190757_create_embeddable_hosts.rb` around lines 24 - 26,
The INSERT currently interpolates host values inside the raw SQL in the
records.each block (the execute call), which allows SQL injection for values
with quotes; fix it by quoting the host value via
ActiveRecord::Base.connection.quote (or connection.quote) before building the
SQL string and use that quoted value in the execute call so the SQL is safe and
handles embedded quotes.
| Fabricator(:embeddable_host) do | ||
| host "eviltrout.com" | ||
| category | ||
| end |
There was a problem hiding this comment.
Fabricator file name doesn't match its content.
category_fabricator.rb now defines Fabricator(:embeddable_host) while embeddable_host_fabricator.rb defines the :category fabricators. The file names and their contents appear to be swapped — category_fabricator.rb should contain category fabricators and embeddable_host_fabricator.rb should contain the embeddable host fabricator. This will confuse anyone looking for either fabricator by convention.
🤖 Prompt for AI Agents
In `@spec/fabricators/category_fabricator.rb` around lines 1 - 4, The file names
and contents are swapped: spec/fabricators/category_fabricator.rb currently
defines Fabricator(:embeddable_host) while
spec/fabricators/embeddable_host_fabricator.rb defines the :category
fabricators; fix this by moving the Fabricator(:embeddable_host) block into
embeddable_host_fabricator.rb and moving the :category fabricators into
category_fabricator.rb (or rename files to match the contents), ensuring
Fabricator(:embeddable_host) lives in embeddable_host_fabricator.rb and the
:category Fabricator(s) live in category_fabricator.rb.
| SiteSetting.embed_truncate = false | ||
| expect(topic.expandable_first_post?).to eq(false) | ||
| end | ||
| describe 'with an emeddable host' do |
There was a problem hiding this comment.
Typo: "emeddable" → "embeddable".
- describe 'with an emeddable host' do
+ describe 'with an embeddable host' do📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe 'with an emeddable host' do | |
| describe 'with an embeddable host' do |
🤖 Prompt for AI Agents
In `@spec/models/topic_spec.rb` at line 1407, The test description string in the
describe block currently contains a typo: change "with an emeddable host" to
"with an embeddable host" in the describe(...) line in spec/models/topic_spec.rb
so the spec reads 'describe "with an embeddable host" do'.
| this.get('/fruits/:id', function() { | ||
| const fruit = fruits[0]; | ||
|
|
||
| return response({ __rest_serializer: "1", fruit, farmers: [farmers[0]] }); | ||
| return response({ __rest_serializer: "1", fruit, farmers, colors }); | ||
| }); |
There was a problem hiding this comment.
/fruits/:id always returns fruits[0] regardless of the requested ID.
The handler ignores the :id param and always returns fruits[0] (id: 1, apple, color_ids: [1,2]). The test in store-test.js.es6 calls store.find('fruit', 2) but actually receives fruit id 1. The assertions (e.g., colors.length === 2) pass only because they match apple's data, not banana's (color_ids: [3]). This makes the test misleading — it appears to test fruit 2 but actually validates fruit 1.
Consider looking up the correct fruit by the requested ID:
Proposed fix
this.get('/fruits/:id', function(request) {
- const fruit = fruits[0];
- return response({ __rest_serializer: "1", fruit, farmers, colors });
+ const fruit = fruits.findBy('id', parseInt(request.params.id));
+ return response({ __rest_serializer: "1", fruit, farmers, colors });
});Then update the test to assert data consistent with the requested fruit ID.
🤖 Prompt for AI Agents
In `@test/javascripts/helpers/create-pretender.js.es6` around lines 226 - 229, The
GET handler for '/fruits/:id' always returns fruits[0] instead of the requested
record; change the handler function signature to accept the request, extract the
id from request.params (e.g. const id = parseInt(request.params.id, 10)), find
the matching fruit (e.g. const fruit = fruits.find(f => f.id === id) ||
fruits[0]) and return that in response({ __rest_serializer: "1", fruit, farmers,
colors }); then update the corresponding test assertions in store-test.js.es6 to
expect the data for the requested fruit ID (e.g. id 2) rather than apple's data.
| test('find embedded', function(assert) { | ||
| const store = createStore(); | ||
| return store.find('fruit', 1).then(function(f) { | ||
| ok(f.get('farmer'), 'it has the embedded object'); | ||
| ok(f.get('category'), 'categories are found automatically'); | ||
| return store.find('fruit', 2).then(function(f) { | ||
| assert.ok(f.get('farmer'), 'it has the embedded object'); | ||
|
|
||
| const fruitCols = f.get('colors'); | ||
| assert.equal(fruitCols.length, 2); | ||
| assert.equal(fruitCols[0].get('id'), 1); | ||
| assert.equal(fruitCols[1].get('id'), 2); | ||
|
|
||
| assert.ok(f.get('category'), 'categories are found automatically'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test requests fruit id 2 but receives and validates fruit id 1.
store.find('fruit', 2) hits the pretender which always returns fruits[0] (id: 1, apple). The assertions on lines 115-117 (colors.length === 2, ids 1 and 2) match apple's color_ids: [1, 2], not banana's color_ids: [3]. If the pretender is later fixed to return the correct fruit by ID, this test will break.
Either change the find call to store.find('fruit', 1) to be explicit, or fix the pretender handler to look up the correct fruit (as noted in the pretender review).
Quick fix: align the test with what the mock actually returns
- return store.find('fruit', 2).then(function(f) {
+ return store.find('fruit', 1).then(function(f) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('find embedded', function(assert) { | |
| const store = createStore(); | |
| return store.find('fruit', 1).then(function(f) { | |
| ok(f.get('farmer'), 'it has the embedded object'); | |
| ok(f.get('category'), 'categories are found automatically'); | |
| return store.find('fruit', 2).then(function(f) { | |
| assert.ok(f.get('farmer'), 'it has the embedded object'); | |
| const fruitCols = f.get('colors'); | |
| assert.equal(fruitCols.length, 2); | |
| assert.equal(fruitCols[0].get('id'), 1); | |
| assert.equal(fruitCols[1].get('id'), 2); | |
| assert.ok(f.get('category'), 'categories are found automatically'); | |
| }); | |
| }); | |
| test('find embedded', function(assert) { | |
| const store = createStore(); | |
| return store.find('fruit', 1).then(function(f) { | |
| assert.ok(f.get('farmer'), 'it has the embedded object'); | |
| const fruitCols = f.get('colors'); | |
| assert.equal(fruitCols.length, 2); | |
| assert.equal(fruitCols[0].get('id'), 1); | |
| assert.equal(fruitCols[1].get('id'), 2); | |
| assert.ok(f.get('category'), 'categories are found automatically'); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In `@test/javascripts/models/store-test.js.es6` around lines 109 - 121, The test
"find embedded" calls store.find('fruit', 2) but the pretender mock always
returns fruits[0] (id 1), so update the test or the mock: either change the
test's call to store.find('fruit', 1) so it matches the current mock response,
or modify the pretender handler that serves the /fruits endpoint to look up the
requested id and return the matching entry from the fruits array; adjust
references in the test around colors and category only if you change the
returned fruit.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Release Notes